Conversation
Xref https://docs.zizmor.sh/audits/#artipacked. Set `persist-credentials: true` when `EndBug/add-and-commit` is used.
|
I will automatically update this comment whenever this PR is modified
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development #756 +/- ##
============================================
Coverage 77.22% 77.22%
============================================
Files 42 42
Lines 3231 3231
Branches 401 401
============================================
Hits 2495 2495
Misses 603 603
Partials 133 133 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
weiji14
left a comment
There was a problem hiding this comment.
Ok, have fixed most issues except for one that is more complicated. See below.
| @@ -2,10 +2,15 @@ name: AddBinderBadge | |||
| on: | |||
| pull_request_target: | |||
There was a problem hiding this comment.
Secure way would be to use pull_request instead of pull_request_target trigger. E.g. following https://mybinder.readthedocs.io/en/latest/howto/gh-actions-badges.html#example-2-comment-with-a-binder-badge-in-response-to-a-comment
Yes, slightly more complicated, but I've done it before at pangeo-data/pangeo-docker-images#631. This will require a change in behaviour, in that someone (with the proper permissions) has to write a comment with /binder to have the Binder button show up. I can go ahead with this if that's ok?
There was a problem hiding this comment.
This sounds like a good approach to me. I'm not sure how often folks use the binder button anyway, so I don't think explicitly needing to comment to ask for it is a bad idea.
There was a problem hiding this comment.
Ok, have modified the workflow in commit 4dcca1c, so someone will need to write /binder to place the Binder badge. Let me know if you want this to be documented somewhere (I was thinking of putting it in the Pull Request template, but there isn't any yet).
| workflow_run: | ||
| workflows: [Update UML diagrams] |
There was a problem hiding this comment.
This workflow_run trigger is apparently dangerous, but also I'm not too sure how to avoid it 🙃. I think the key is that we want the unit tests to be re-ran after the UML diagram commit, need to think of how it can be done more safely.
There was a problem hiding this comment.
The reason we need to run the unit tests after the UML diagram commit is because the unit tests passing are required branch protection checks for merging. Is there a better approach to getting the UML diagrams updated that makes it so we don't need this workflow trigger to rerun the unit tests? Otherwise I'm also not sure how to avoid this trigger.
There was a problem hiding this comment.
Ok, so reading your previous comment at #618 (comment) which links to https://github.com/EndBug/add-and-commit/tree/v10.0.0#the-commit-from-the-action-is-not-triggering-ci, they say:
If you're sure that you want the commits generated during CI to trigger other workflow runs, you can checkout the repo using a Personal Access Token (PAT): this will make the resulting commit the same as if you made it yourself.
So I could make sure the commit happens with a Personal Access Token (PAT) instead of the default GITHUB_TOKEN, and then we should be able to remove the workflow_run trigger. I had a look around and there seems to be one already used by the traffic action here:
icepyx/.github/workflows/traffic_action.yml
Lines 27 to 28 in 31e208a
So we could probably just re-use that token (if that's ok, or we can create another one). How does that sound?
There was a problem hiding this comment.
So we could probably just re-use that token (if that's ok, or we can create another one). How does that sound?
By the way, if deciding to create a new token, I'd recommend adding the secret PAT to a dedicated environment (see: creating an enviroment) so that it can only be accessed in a restricted way. Downside is that a maintainer will need to manually approve the UML workflow run (in addition to approving the PR itself). Up to you on what your risk tolerance is (we'll actually be fairly safe already after this PR's changes).
Follow up to #754 to apply more security related fixes.
Will need to check https://github.com/icesat2py/icepyx/security/code-scanning?query=is%3Aopen+branch%3Adevelopment+tool%3Azizmor (after this PR is merged into the
developmentbranch) to ensure all issues have been resolvedTODO: